-
Notifications
You must be signed in to change notification settings - Fork 791
[SYCL] Implement sycl_khr_includes
extension
#20339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: sycl
Are you sure you want to change the base?
[SYCL] Implement sycl_khr_includes
extension
#20339
Conversation
61f1267
to
9011b04
Compare
9011b04
to
9d73285
Compare
// a version. | ||
// However, we may need to have some fallback here if someone includes SYCL | ||
// headers into their host applications and use 3rd-party compilers. | ||
// #error "SYCL_LANGUAGE_VERSION is not defined, please report this as a bug" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing happens here. do we need this dead code and comment then? I suggest to add it when it will be needed and will do something depending on condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those lines worth discussion, I think.
My original intent was to add a guardrail about us refactoring the compiler, but forgetting to define it in headers (or rather defining it in a header other than version.hpp
).
However, that attempt failed because apparently we ourselves have an application (sycl-ls
) that is built with a 3rd-party compiler (i.e. host gcc
or whatever), but uses SYCL headers.
Do we want our headers to be usable with 3rd-party compilers? By that I mean directly using g++ -I/path/to/sycl/includes -include sycl.hpp
instead of clang++ --fsycl-host-compiler=g++
. We actually have a feature request about this: #5932
If we do want such support, then we should probably define SYCL_LANGUAGE_VERSION
macro here. If not, then #error
is the right way to go and we need to fix how we build sycl-ls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have a warning for this case https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/sycl.hpp#L27
that shows absence of guarantee and doesn't block usage by 3rd party compiler if someone want to try. Why can't we stick to the same approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reminding me about that diagnostic. If we explicitly claim it to be erroneous, then I would remove the #ifdef
completely, providing no guarantee of SYCL_LANGUAGE_VERSION
being defined in such mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The specification can be found in KhronosGroup/SYCL-Docs#814
The implementation is a non-functional change for the existing codebase, but definitions of macro documented by the SYCL 2020 specifications were moved around a little bit to provide them all through the new
sycl/khr/version.hpp
.New headers are always present and their content is always available. However, we do not define
SYCL_KHR_INCLUDES
macro yet because the extension is still on review. That indicates that it is not formally supported by the implementation.